Hala/Leah (Space) & Hannah/Jocelyn (Time)#28
Hala/Leah (Space) & Hannah/Jocelyn (Time)#28wangjoc wants to merge 378 commits intoAda-C13:masterfrom
Conversation
adds footer links back in
…r can both access it
…k or card-columns
…der only if there are orders
…e damned get in touch link to be the right f-ing color
bEtsyFunctional Requirements: Manual Testing
Major Learning Goals/Code Review
Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
Overall FeedbackOnly the person who submitted the PR will get an email about this feedback. Please let the rest of your team know about it. |
There was a problem hiding this comment.
Hala - Your seeded data is AMAZING. I'm not sure what suggestions I have around the categories_products join table. It looks good to me! Happy to have a conversation about it.
Hannah - The testing is quite thorough. There are a couple failure cases that are missing tests that are shown by simple cov (ex. Merchant_controller: could not create a new user account, Product model and product: search).
Leah - The user experience was pretty flawless. I commented on one user experience issue in the 'retire product' feature. There is no indication that the product is retired -- how do you un-retire it?
Jocelyn - Ack, sorry! I didn't read this comment until after I left comments about business logic in the controller. I understand now why this logic is in the controller. When you're dealing with session it makes sense to put that logic in the controller. Sometimes you can write controller helper methods if it's getting overly businessy. Looking back, it still seems that the order revenue calculation could be model method. Correct me if I'm wrong. Happy to talk about it more!
| end | ||
|
|
||
| def confirm | ||
| @order = Order.find_by(id: session[:order_id]) |
There was a problem hiding this comment.
It looks like you've doubled up your work with a find_order controller filter and this code.
| return | ||
| end | ||
|
|
||
| @order_revenue = 0 |
There was a problem hiding this comment.
This logic looks "businessy". Could you do some of this work in a custom model method?
| @order = Order.find_by(id: session[:order_id]) | ||
|
|
||
| @order_revenue = 0 | ||
| @order.order_items.each do |order_item| |
There was a problem hiding this comment.
This code looks just like what you have on line 34. Putting this in a model method would help DRY up you code.
| return params.require(:order).permit(:buyer_name, :mail_address, :zip_code, :email_address, :cc_num, :cc_exp, :cc_cvv) | ||
| end | ||
|
|
||
| def fix_params |
There was a problem hiding this comment.
Again this seems pretty businessy. I wonder if it could be put in the model.
| @@ -0,0 +1,49 @@ | |||
| <% # DISPLAYS ALL PRODUCTS, REGARDLESS OF MERCHANT %> | |||
|
|
|||
| <h1 class='title'>Products Results</h1> | |||
There was a problem hiding this comment.
Consider whether you might be able to use a partial view for these 3 chunks of code that are quite similar (products, categories, and merchants)
Assignment Submission: bEtsy
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions. These should be answered by all members of your team, not by a single teammate.
Reflection